Is it fair to say that this new unified mux is most similar to the existing NewGrpcMuxImpl? If a reviewer were to compare the two side-by-side, what would be the key differences or things to pay attention to?

Is it fair to say that this new unified mux is most similar to the existing NewGrpcMuxImpl.

Definitely. Will add some pointers here a bit later.

• The most significant source of difference between the implementations is introduction of templates; most of the time there's little to none difference in logic in methods.
• There's no SubscriptionStuff in the unified mux implementation; b/c of that creating/accessing subscription state is a bit different.
• Both delta and sotw muxes in the new implementation respect skip_subsequent_node flag; the flag is used insendGrpcMessage() method.
• Concrete mux classes are tiny, pretty much all of the logic is in the base class.

Thanks, very helpful, CC @adisuissa

nit: implement these directly in the header file

We now have a check that yells if exceptions are being raised in headers, which makes me think that such an approach should be used sparingly?

Yeah, no exception throwing in headers :) @adisuissa we have an internal requirement around this.

Why is this not implemented and the other methods throw an exception (ADS must be configured)?

Hmm, I don't feel strongly about this; my thinking was that requestOnDemandUpdate only makes sense for a delta mux, so all other implementations panic. I can throw an exception here if you think it's easier to understand/read.

We shouldn't assume that delta implies ADS though if that's what is suggested.

I do think the original message in the exception is weird (see also here: https://github.com/envoyproxy/envoy/blob/main/source/common/config/grpc_mux_impl.h#L215). Perhaps a simple panic would suffice in those places? I could a separate PR to update the message in legacy mux implementation.

It may be better to only cast in the if clause, and then apply the ->grpcStreamForTest().onRemoteClose(Grpc::Status::WellKnownGrpcStatus::Canceled, ""); outside of the if clause. (Should improve maintenance down the road).
Also applies to the functions below.

Not sure what you mean here: grpcStreamForTest() isn't in the mux interface, concrete mux types are needed to invoke it. There isn't a common base between XdsMux::GrpcMuxDelta and NewGrpcMuxImpl, I can't declare a local var to hold the result of such cast...

Is this due to an issue in the legacy-delta implementation? Is there an issue# to add to this?

Nods, In NewGrpcMuxImpl addWatch call returns a wrapper a Watch, which also has a dtor (here: https://github.com/envoyproxy/envoy/blob/main/source/common/config/new_grpc_mux_impl.h#L114) that's being called when foo_sub goes out of scope, and an unsubscribe message is sent. Unified mux implementation returns a thin wrapper around a Watch that's there for compatibility with existing interface only (So I don't need to introduce a very similar addWatch method). This compatibility wrapper doesn't have perform an unsubscribe on destruction, hence the difference.

Sorry, coming in late, why did this need a coverage exemption?